-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test/aggmds counts #1064
base: master
Are you sure you want to change the base?
Test/aggmds counts #1064
Conversation
src/Discovery/Discovery.tsx
Outdated
@@ -89,7 +89,7 @@ export const renderFieldContent = (content: any, contentType: 'string'|'paragrap | |||
if (Array.isArray(content)) { | |||
return content.join(', '); | |||
} | |||
return content.toLocaleString(); | |||
return content ? content.toLocaleString() : 'N/A'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice I like this as this won't crash undef values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions: does a missing content
value need to be type matched? eg: if an int is missing the portal code should be looking for 0 as opposed to N/A
? also how about __manifest
? If there is none, that should be [] as opposed to N/A
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I have updated the expression
src/Discovery/Discovery.tsx
Outdated
@@ -89,7 +89,7 @@ export const renderFieldContent = (content: any, contentType: 'string'|'paragrap | |||
if (Array.isArray(content)) { | |||
return content.join(', '); | |||
} | |||
return content.toLocaleString(); | |||
return content ? content.toLocaleString() : 'N/A'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use valueIfNotAvailable to handle missing values? Just like what we were doing in existing codes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works, however, it requires the configuration to ensure all fields are using valueIfNotAvailable
But that is cleaner solution
@craigrbarnes is this still needed? |
Yes. I'm looking at it now. It is needed for BRH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
Jira Ticket: BRH-169
New Features
__manifest
field, it adds the count option to the aggregate MDS which returns the length of the array. If a study is selected the array contents of the__manifest
field is pulled from the aggreateMDS.Breaking Changes
Bug Fixes
_subject_count
is not a number, change the value to "N/A".Improvements
Dependency updates
Deployment changes